Skip to content

Fix bad format string in ext_authz trace log#18492

Merged
rojkov merged 3 commits intoenvoyproxy:mainfrom
dyfrgi:fix-authz-log
Oct 12, 2021
Merged

Fix bad format string in ext_authz trace log#18492
rojkov merged 3 commits intoenvoyproxy:mainfrom
dyfrgi:fix-authz-log

Conversation

@dyfrgi
Copy link
Copy Markdown
Contributor

@dyfrgi dyfrgi commented Oct 6, 2021

Signed-off-by: Michael Leuchtenburg mleuchtenburg@google.com

Commit Message: Fixes a broken log line. This message currently doesn't appear because the format is wrong.
Additional Description: This is hard to detect because Envoy doesn't use FMT_STRING, which would do compile-time checking of formats. It should be logged to stderr by default by spdlog, but I'm not seeing those logs anywhere when running the ext_authz tests.
Risk Level: low
Testing: Ran the ext_authz tests, verified that the log message appears.

Signed-off-by: Michael Leuchtenburg <mleuchtenburg@google.com>
@dyfrgi dyfrgi requested a review from dio as a code owner October 6, 2021 18:48
Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. And added a minor comment.

Comment on lines 163 to 164
ENVOY_STREAM_LOG(trace,
"ext_authz filter has {} response header(s) to add and {} response header(s) to "
"set to the encoded response:",
"ext_authz filter has {} response header(s) to add",
*encoder_callbacks_, response_headers_to_add_.size());
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be we can just add response_headers_to_set_.size().

ENVOY_STREAM_LOG(trace,
                 "ext_authz filter has {} response header(s) to add and {} response header(s) to "
                 "set to the encoded response:", 
                 *encoder_callbacks_, response_headers_to_add_.size(), response_headers_to_set_.size());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to adding the missing argument.

Copy link
Copy Markdown
Contributor Author

@dyfrgi dyfrgi Oct 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, good call. Done.

EDIT: Oh and confirmed that the log line does now appear when running ext_authz_test.
[2021-10-07 15:33:39.952][12][trace][filter] [source/extensions/filters/http/ext_authz/ext_authz.cc:162] [C0][S0] ext_authz filter has 1 response header(s) to add and 1 response header(s) to set to the encoded response:

@rojkov rojkov self-assigned this Oct 7, 2021
Signed-off-by: Michael Leuchtenburg <mleuchtenburg@google.com>
@rojkov
Copy link
Copy Markdown
Member

rojkov commented Oct 8, 2021

/wait

@dyfrgi dyfrgi changed the title Remove unused portion of log message with bad format string Fix bad format string in ext_authz trace log Oct 8, 2021
@rojkov
Copy link
Copy Markdown
Member

rojkov commented Oct 11, 2021

Thank you! Looks very good. The format precheck can be fixed with ./tools/code_format/check_format.py fix.

I'd recommend installing the git hooks with ./support/bootstrap to do prechecks automatically upon git push.

@rojkov
Copy link
Copy Markdown
Member

rojkov commented Oct 11, 2021

/wait

Signed-off-by: Michael Leuchtenburg <mleuchtenburg@google.com>
@dyfrgi
Copy link
Copy Markdown
Contributor Author

dyfrgi commented Oct 11, 2021

Thanks! I'd have sworn that I did install the hooks with ./support/bootstrap, but now I see that I just linked prepare-commit-message, not pre-push. Too bad that script can't work on just changed files - it suggested 3 changes to files I didn't modify.

Copy link
Copy Markdown
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks.

Copy link
Copy Markdown
Member

@dio dio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Copy link
Copy Markdown
Member

@rojkov rojkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@rojkov rojkov merged commit 2110d03 into envoyproxy:main Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants